-
Notifications
You must be signed in to change notification settings - Fork 2k
[CHORE] Fix the MCMR memberlists in Tilt. #6099
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
|
Tilt updates to grant rust frontend memberlist access Adds missing Key Changes• Added Affected Areas• Tiltfile This summary was automatically generated by @propel-code-bot |
| 'rust-frontend-service-serviceaccount:ServiceAccount:chroma', | ||
| 'rust-frontend-service-rolebinding:RoleBinding:chroma', | ||
| 'rust-frontend-service-query-service-memberlist-binding:RoleBinding:chroma', | ||
| 'rust-log-service-memberlist-readerwriter:Role:chroma', | ||
| 'rust-frontend-service-rust-log-service-memberlist-binding:RoleBinding:chroma', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Maintainability] These new resource definitions are also duplicated for the chroma2 namespace below (lines 314-318). The entire objects list for k8s_setup and k8s_setup2 is almost identical.
To avoid future inconsistencies and make this file easier to maintain, consider abstracting the resource list into a function that takes the namespace as an argument. This would centralize the resource definitions and ensure both environments stay in sync.
For example:
def get_k8s_setup_objects(namespace):
return [
'pod-watcher:Role:{}'.format(namespace),
# ... (rest of the common resources)
'rust-frontend-service-serviceaccount:ServiceAccount:{}'.format(namespace),
'rust-frontend-service-rolebinding:RoleBinding:{}'.format(namespace),
'rust-frontend-service-query-service-memberlist-binding:RoleBinding:{}'.format(namespace),
'rust-log-service-memberlist-readerwriter:Role:{}'.format(namespace),
'rust-frontend-service-rust-log-service-memberlist-binding:RoleBinding:{}'.format(namespace),
# ...
]
k8s_resource(
objects=get_k8s_setup_objects('chroma'),
new_name='k8s_setup',
labels=["infrastructure"],
)
k8s_resource(
objects=get_k8s_setup_objects('chroma2'),
new_name='k8s_setup2',
labels=["infrastructure2"],
)Applying this pattern would make adding or removing resources for both environments a single-line change.
Context for Agents
These new resource definitions are also duplicated for the `chroma2` namespace below (lines 314-318). The entire `objects` list for `k8s_setup` and `k8s_setup2` is almost identical.
To avoid future inconsistencies and make this file easier to maintain, consider abstracting the resource list into a function that takes the namespace as an argument. This would centralize the resource definitions and ensure both environments stay in sync.
For example:
```python
def get_k8s_setup_objects(namespace):
return [
'pod-watcher:Role:{}'.format(namespace),
# ... (rest of the common resources)
'rust-frontend-service-serviceaccount:ServiceAccount:{}'.format(namespace),
'rust-frontend-service-rolebinding:RoleBinding:{}'.format(namespace),
'rust-frontend-service-query-service-memberlist-binding:RoleBinding:{}'.format(namespace),
'rust-log-service-memberlist-readerwriter:Role:{}'.format(namespace),
'rust-frontend-service-rust-log-service-memberlist-binding:RoleBinding:{}'.format(namespace),
# ...
]
k8s_resource(
objects=get_k8s_setup_objects('chroma'),
new_name='k8s_setup',
labels=["infrastructure"],
)
k8s_resource(
objects=get_k8s_setup_objects('chroma2'),
new_name='k8s_setup2',
labels=["infrastructure2"],
)
```
Applying this pattern would make adding or removing resources for both environments a single-line change.
File: Tiltfile
Line: 274d901714 to
368def8
Compare
43c76c7 to
c150db7
Compare
rust/spanner-migrations/log_migrations/0002-create_fragments_table.spanner.sql
Outdated
Show resolved
Hide resolved
rust/spanner-migrations/log_migrations/0001-create_manifests_table.spanner.sql
Outdated
Show resolved
Hide resolved
rust/spanner-migrations/log_migrations/0003-create_fragment_regions_table.spanner.sql
Outdated
Show resolved
Hide resolved
368def8 to
2e30f26
Compare
sanketkedia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm my understanding - this isn't MCMR specific right? FE in chroma namespace also has the same issue when accessing query service memberlist for e.g.?
| CREATE TABLE IF NOT EXISTS fragments ( | ||
| log_id STRING(36) NOT NULL, | ||
| ident STRING(36) NOT NULL, | ||
| path STRING(64) NOT NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Logic] The path column is defined as STRING(64), which may be too short for fragment paths generated with sequence numbers.
The path format for sequence-numbered fragments is log/Bucket={:016x}/FragmentSeqNo={:016x}.parquet, which results in a 66-character string (e.g., log/Bucket=0000000000000000/FragmentSeqNo=0000000000000001.parquet). This will cause an error when inserting into the fragments table.
Consider increasing the size of the path column to accommodate these longer paths, for example, STRING(128).
Context for Agents
The `path` column is defined as `STRING(64)`, which may be too short for fragment paths generated with sequence numbers.
The path format for sequence-numbered fragments is `log/Bucket={:016x}/FragmentSeqNo={:016x}.parquet`, which results in a 66-character string (e.g., `log/Bucket=0000000000000000/FragmentSeqNo=0000000000000001.parquet`). This will cause an error when inserting into the `fragments` table.
Consider increasing the size of the `path` column to accommodate these longer paths, for example, `STRING(128)`.
File: rust/spanner-migrations/log_migrations/0002-create_fragments_table.spanner.sql
Line: 4There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's log-uuid that gets used here. log/Uuid={}.parquet fits a 36-char UUID in 64 chars.
2e30f26 to
093f00b
Compare
093f00b to
07459d5
Compare
Description of changes
The frontend was failing to come up. Extend permissions to access memberlist.
Test plan
CI
Migration plan
N/A
Observability plan
N/A
Documentation Changes
N/A